Changes to SUNDIALS API and core C/C++ needed for Python interfaces #768

balos1 · 2025-09-20T00:06:23Z

This is the changes that are needed to enable the Python interfaces without the bindings themselves included to make the review process easier.

…r constant expression folding

balos1 · 2025-09-22T17:34:50Z

@gardner48 When this is merged into the feature/python-interfaces branch, I don't want the commits squashed so that I don't get a bunch of merge conflicts between this part1 branch and part2.

Steven-Roberts

Made it through docs and include

For sunrealtype1d and similar typedefs, do we want to use them in docs or code only?
Changelog needs updating

Steven-Roberts · 2025-09-23T15:42:50Z

include/arkode/arkode.hpp

+ * Programmer(s): Cody J. Balos @ LLNL
+ * -----------------------------------------------------------------------------
+ * SUNDIALS Copyright Start
+ * Copyright (c) 2002-2024, Lawrence Livermore National Security


Suggested change

* Copyright (c) 2002-2024, Lawrence Livermore National Security

* Copyright (c) 2002-2025, Lawrence Livermore National Security

I'm surprised that this wasn't caught automatically by the copyright script.

Steven-Roberts · 2025-09-23T16:14:28Z

include/arkode/arkode.hpp

+{
+  void operator()(ARKodeButcherTable t)
+  {
+    if (t) { ARKodeButcherTable_Free(t); }


Suggested change

if (t) { ARKodeButcherTable_Free(t); }

ARKodeButcherTable_Free(t);

Steven-Roberts · 2025-09-23T16:25:13Z

include/arkode/arkode_mristep.h

                                                   sunrealtype t, N_Vector f);
 SUNDIALS_EXPORT int MRIStepInnerStepper_GetForcingData(
  MRIStepInnerStepper stepper, sunrealtype* tshift, sunrealtype* tscale,
  N_Vector** forcing, int* nforcing);


Suggested change

N_Vector** forcing, int* nforcing);

N_Vector1d* forcing, int* nforcing);

Steven-Roberts · 2025-09-23T16:25:29Z

include/arkode/arkode_mristep.hpp

+ * Programmer(s): Cody J. Balos @ LLNL
+ * -----------------------------------------------------------------------------
+ * SUNDIALS Copyright Start
+ * Copyright (c) 2002-2024, Lawrence Livermore National Security


Suggested change

* Copyright (c) 2002-2024, Lawrence Livermore National Security

* Copyright (c) 2002-2025, Lawrence Livermore National Security

Steven-Roberts · 2025-09-23T16:26:31Z

include/arkode/arkode_mristep.hpp

+{
+  void operator()(MRIStepCoupling t)
+  {
+    if (t) { MRIStepCoupling_Free(t); }


Suggested change

if (t) { MRIStepCoupling_Free(t); }

MRIStepCoupling_Free(t);

Steven-Roberts · 2025-09-23T17:20:07Z

include/sundials/sundials_errors.h


 /* clang-format off */
-enum
+enum SUNErrCode_


Does this need some swig #ifdef that was used in a few other places?

Steven-Roberts · 2025-09-23T17:22:21Z

include/sundials/sundials_iterative.h

 */

-enum
+enum SUN_GRAMSCHMIDT_ID


Same question about swig ifdef

Steven-Roberts · 2025-09-23T17:24:32Z

include/sundials/sundials_logger.hpp

+namespace sundials {
+namespace experimental {
+
+class SUNLoggerView : public sundials::ConvertibleTo<SUNLogger>


Same question about inheriting from ClassView. Would that require a major release?

Steven-Roberts · 2025-09-23T17:25:10Z

include/sundials/sundials_logger.hpp

+  }
+
+private:
+  std::unique_ptr<SUNLogger> logger_;


Same comment about using deleter template argument

Steven-Roberts · 2025-09-23T17:28:07Z

include/sundials/sundials_nvector.h

 typedef _SUNDIALS_STRUCT_ _generic_N_Vector* N_Vector;

 /* Define array of N_Vectors */
 typedef N_Vector* N_Vector_S;


Could we replace usages of N_Vector_S with N_Vector1d?

drreynolds

This looks good. I had a few questions, but I think these are more due to my lack of understanding than this needing corrections.

drreynolds · 2025-09-24T00:54:00Z

doc/shared/sundials/Fortran.rst


 .. c:function:: SUNErrCode SUNDIALSFileClose(FILE** fp)

+   Deprecated alias to :c:func:`SUNFileOpen`


Suggested change

Deprecated alias to :c:func:`SUNFileOpen`

Deprecated alias to :c:func:`SUNFileClose`

drreynolds · 2025-09-24T00:55:34Z

doc/shared/nvectors/NVector_Kokkos.rst


-   .. cpp:function:: N_Vector Convert() override
+   .. cpp:function:: N_Vector get() override

      Explicit conversion to a :c:type:`N_Vector`.

-   .. cpp:function:: N_Vector Convert() const override
+   .. cpp:function:: N_Vector get() const override



Do these constitute a breaking change? What was the need for changing the member function name?

Yes, this would be a breaking change. If the name change is necessary, we'll need to also provide Convert until the next major release.

drreynolds · 2025-09-24T00:56:29Z

doc/shared/sundials/Fortran.rst

+   Deprecated alias to :c:func:`SUNFileOpen`.
+
+
+.. c:function:: SUNErrCode SUNFileOpen(const char* filename, const char* mode, FILE** fp)
+


Can you comment on the need to change SUNDIALSFileOpen and SUNDIALSFileClose to SUNFileOpen and SUNFileClose for this PR?

drreynolds · 2025-09-24T00:57:45Z

doc/shared/sundials/SUNContext.rst

-   SUNContext Convert() override
+   SUNContext get() override
   {
      return *sunctx_.get();
   }
-   SUNContext Convert() const override
+   SUNContext get() const override


Again, is this a breaking change? Why did the member function name need to be changed?

drreynolds · 2025-09-24T01:00:37Z

doc/shared/sunlinsol/SUNLinSol_Ginkgo.rst

-   .. cpp:function:: SUNLinearSolver Convert() override
+   .. cpp:function:: SUNLinearSolver get() override

      Explicit conversion to a :c:type:`SUNLinearSolver`.

-   .. cpp:function:: SUNLinearSolver Convert() const override
+   .. cpp:function:: SUNLinearSolver get() const override


Breaking change? Why the name change?

drreynolds · 2025-09-24T01:03:01Z

include/arkode/arkode.hpp

+ * Programmer(s): Cody J. Balos @ LLNL
+ * -----------------------------------------------------------------------------
+ * SUNDIALS Copyright Start
+ * Copyright (c) 2002-2024, Lawrence Livermore National Security


I'm surprised that this wasn't caught automatically by the copyright script.

drreynolds · 2025-09-24T01:09:08Z

include/arkode/arkode_mristep.h

-typedef int (*MRIStepPreInnerFn)(sunrealtype t, N_Vector* f, int nvecs,
+typedef int (*MRIStepPreInnerFn)(sunrealtype t, N_Vector1d f, int nvecs,


doc/shared/sundials/Types.rst introduced sunrealtypeNd and sunindextypeNd, but it never mentioned N_Vector1d -- should that be added?

drreynolds · 2025-09-24T01:13:16Z

include/cvodes/cvodes.h

+SUNDIALS_EXPORT int CVodeSetSensParams(void* cvode_mem, sunrealtype1d p,
+                                       sunrealtype1d pbar, int1d plist);


doc/shared/sundials/Types.rst introduced sunindextype1d but not int1d -- should this be added to the documentation?

drreynolds · 2025-09-24T01:16:53Z

include/nvector/nvector_kokkos.hpp

-  N_Vector Convert() override { return object_.get(); }
+  N_Vector get() override { return object_.get(); }

-  N_Vector Convert() const override { return object_.get(); }
+  N_Vector get() const override { return object_.get(); }


Breaking change? Why the name change?

drreynolds · 2025-09-24T01:22:00Z

include/sundials/sundials_base.hpp

-  T Convert() override { return *object_.get(); }
+  T get() override { return object_; }

-  T Convert() const override { return *object_.get(); }
+  T get() const override { return object_; }


I'm guessing that this is the cause for the member function changes elsewhere in our C++ headers. I don't fully follow the need for this, but I'm ready to take your word on it.

balos1 added 6 commits September 19, 2025 16:26

pick changes to sundials C/C++ interface or other core sundials

17c89c2

document API changes

683cb92

pick gitignore

431905b

doc update

5213a1e

move SetOwnUserData functions to impl

1c40de4

doc some types

0352562

balos1 requested review from gardner48, cswoodward and drreynolds as code owners September 20, 2025 00:06

balos1 added 3 commits September 22, 2025 10:21

use macro defines for constants instead of const int to enable intege…

ae7adee

…r constant expression folding

use SUN_DATAIOMODE_

915394e

use SUN_MEMTYPE

112b638

balos1 added dont-merge new-feature labels Sep 22, 2025

newline

7e6669b

balos1 changed the base branch from develop to feature/python-interfaces September 22, 2025 17:33

balos1 removed the dont-merge label Sep 22, 2025

balos1 added 12 commits September 22, 2025 10:36

remove duplicate typedef from merge conflict

56711a6

regen swig

9df8b5a

no anonymous enums

b5d8c19

move SPRKStepSetUseCompensatedSums

4e0d03e

undo SUN_MEMTYPE / SUN_DATAIOMODE switch

3fd9b27

find/replace double underscore SUNMEMTYPE__

73fe771

regen fortran

f1de1af

missed Convert to get

67a1dcc

merge error

eba70ba

typo

778359f

add python pointers for linearsolver and nonlinear

dc49482

regen swig after typo

5d94366

balos1 added 4 commits September 22, 2025 23:46

remove enum trailing underscores

abbecce

add _deprecated headers to cmake

c6b2b8b

format

12a66ee

merge fixes

8d373a7

Steven-Roberts reviewed Sep 23, 2025

View reviewed changes

gardner48 added this to the SUNDIALS After Next milestone Sep 24, 2025

drreynolds reviewed Sep 24, 2025

View reviewed changes

	* Copyright (c) 2002-2024, Lawrence Livermore National Security
	* Copyright (c) 2002-2025, Lawrence Livermore National Security

	if (t) { ARKodeButcherTable_Free(t); }
	ARKodeButcherTable_Free(t);

	N_Vector** forcing, int* nforcing);
	N_Vector1d* forcing, int* nforcing);


		.. c:function:: SUNErrCode SUNDIALSFileClose(FILE** fp)

		Deprecated alias to :c:func:`SUNFileOpen`

	Deprecated alias to :c:func:`SUNFileOpen`
	Deprecated alias to :c:func:`SUNFileClose`

		Deprecated alias to :c:func:`SUNFileOpen`.


		.. c:function:: SUNErrCode SUNFileOpen(const char* filename, const char* mode, FILE** fp)

		typedef int (MRIStepPreInnerFn)(sunrealtype t, N_Vector f, int nvecs,
		typedef int (*MRIStepPreInnerFn)(sunrealtype t, N_Vector1d f, int nvecs,

		SUNDIALS_EXPORT int CVodeSetSensParams(void* cvode_mem, sunrealtype1d p,
		sunrealtype1d pbar, int1d plist);

Changes to SUNDIALS API and core C/C++ needed for Python interfaces #768

Are you sure you want to change the base?

Changes to SUNDIALS API and core C/C++ needed for Python interfaces #768

Conversation

balos1 commented Sep 20, 2025

Uh oh!

balos1 commented Sep 22, 2025

Uh oh!

Steven-Roberts left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

drreynolds left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!